Added a test city (streets and river centerline) to use for testing rcoins #27
Conversation
|
Note that most of the functions are almost directly copy-pasted from the CRiSp package. We may consider factorizing these into their own package (at some point) if they're reused in a number of other packages. |
|
Also note that this dataset stores the city name with the data. The other parts of the data are stored in the geom member. |
There was a problem hiding this comment.
Thanks a lot @vanlankveldthijs, looks good ! Just left few comments, I know they make the code less generic, but for the data script I would actually prefer to have a short and straightforward script..
|
@vanlankveldthijs After discussing it with @cforgaci, we think it makes more sense to call the test data |
There was a problem hiding this comment.
@vanlankveldthijs thanks. I left a few additional comments.
@fnattino fyi
Applied various review suggestions for changes. Co-authored-by: Claudiu Forgaci <[email protected]> Co-authored-by: Francesco Nattino <[email protected]>
|
All previous reviews and code suggestions should now be resolved. |
There was a problem hiding this comment.
Thanks @vanlankveldthijs. I added a few more tiny suggestions. These are needed for the lintr, test and (part of) R-CMD-check actions to pass.
Replaced test_city variable by bucharest Co-authored-by: Claudiu Forgaci <[email protected]>
This reverts commit d57b5d5.
Co-authored-by: Claudiu Forgaci <[email protected]>
There was a problem hiding this comment.
@vanlankveldthijs, many thanks! All checks passing now. @fnattino, if you also agree, this can be merged.
There was a problem hiding this comment.
Thanks a lot @vanlankveldthijs and @cforgaci - a very minor suggestion from my side, then good to merge for me!
environment.yml
Outdated
| @@ -14,3 +14,4 @@ dependencies: | |||
| - r-testthat >=3.0.0 | |||
| # Others | |||
| - r-lintr | |||
| - r-osmdata | |||
There was a problem hiding this comment.
Maybe put r-osmdata right after r-sfnetworks in the list, as it is part of the "Suggests" section?
There was a problem hiding this comment.
Apologies for taking over @vanlankveldthijs , since it was a very minor thing I thought I would fix it quickly. Really thank you so much!
This includes
This also required osmdata to be added to the environment.